Skip to content

feat: expose configuration_access field in PciRoot#233

Merged
qwandor merged 1 commit intorcore-os:masterfrom
AsakuraMizu:pr/deref-pciroot
Mar 18, 2026
Merged

feat: expose configuration_access field in PciRoot#233
qwandor merged 1 commit intorcore-os:masterfrom
AsakuraMizu:pr/deref-pciroot

Conversation

@AsakuraMizu
Copy link
Copy Markdown
Contributor

Description

Added Deref<Target = C> and DerefMut<Target = C> implementations for PciRoot<C>.

We need this to access PCI configuration space (for example Interrupt Line) after constructing a PciRoot. See arceos-org/axdriver_crates#19.

Possible alternatives

  • Add inner() and inner_mut() for PciRoot
  • Implement ConfigurationAccess for PciRoot

@qwandor qwandor self-requested a review March 18, 2026 13:30
@qwandor
Copy link
Copy Markdown
Collaborator

qwandor commented Mar 18, 2026

I'm not convinced that Deref is appropriate here, but I think your suggestion of inner() and inner_mut() sounds reasonable, can we do that?

@AsakuraMizu
Copy link
Copy Markdown
Contributor Author

I'm not convinced that Deref is appropriate here, but I think your suggestion of inner() and inner_mut() sounds reasonable, can we do that?

Actually, I also think Deref is not very good. For example, the behavior of root.unsafe_clone() can cause misunderstandings, but the advantage is that it is easy to use.

Let's talk about inner()/inner_mut(). First, the name: is inner appropriate? Calling it configuration_access()/configuration_access_mut() is clearly too long. Second, this interface is somewhat difficult to use. Users need to call root.[TBD]().read_word() or root.[TBD]_mut().write_word().


However, I've come up with two new alternatives:

  1. Instead of impl ConfigurationAccess for PciRoot, add read_word/write_word methods to PciRoot. This way, users won't accidentally call root.unsafe_clone(), but can still call other two methods as easily as Deref. The downside is the lack of extensibility.

  2. (My preferred approach) Simply change the configuration_access field of PciRoot to pub.

@qwandor
Copy link
Copy Markdown
Collaborator

qwandor commented Mar 18, 2026

The advantage of providing access to the actual ConfigurationAccess implementation (rather than adding methods to PciRoot or implementing ConfigurationAccess for it) is that the specific implementation might have methods which are not part of the trait but which someone might want to call, such as the is_pkvm() method on HypCam. I'm fine with either inner or configuration_access as the name of the method, I don't think the length of the name is a big issue as long as it's clear.

Making the field public is also fine I guess.

@qwandor
Copy link
Copy Markdown
Collaborator

qwandor commented Mar 18, 2026

Actually yeah, I think making the field public is the best solution. It provides exactly the same capabilities as adding the methods, and is simpler.

@AsakuraMizu AsakuraMizu changed the title feat: implement Deref for PciRoot feat: expose configuration_access field in PciRoot Mar 18, 2026
@qwandor qwandor merged commit f5adade into rcore-os:master Mar 18, 2026
5 checks passed
@AsakuraMizu AsakuraMizu deleted the pr/deref-pciroot branch March 20, 2026 07:33
sunhaosheng pushed a commit to sunhaosheng/axdriver_crates that referenced this pull request Mar 31, 2026
- Remove the redundant  parameter from ,
  since  now exposes  as a public field
  (rcore-os/virtio-drivers#233).
- Use  directly on x86_64 instead.
- Add  to .
- Bump  dependency to 0.13.0 (patch via git for the
  public  field).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants